Code cleanups, including rewrite of how FF CLI is handled, and more FF status registers#280
Conversation
There was a problem hiding this comment.
Pull request overview
Adds additional Firefly alarm/status registers to the monitoring address tables and exposes them via new CLI commands, while refactoring Firefly CLI table printing and fixing a channel-disable masking issue.
Changes:
- Extend MON_I2C Rev2/Rev3 YAML to include additional Firefly alarm/status registers (TX fault, RX power, temperature alarm, VCC alarm) and adjust some register metadata.
- Refactor Firefly CLI output into a shared paged “table printer” helper and add new Rev2/Rev3-only CLI commands for the new alarm registers.
- Improve developer ergonomics in the top-level
format/format-applytargets by adding quieter default output with optional verbose logging.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| sm_cm_config/data/MON_I2C_rev3.yml | Adds new Firefly alarm/status registers to Rev3 monitoring config. |
| sm_cm_config/data/MON_I2C_rev2.yml | Adds new Firefly alarm/status registers and fixes CHANNEL_DISABLE mask to 0xFFFF. |
| projects/cm_mcu/commands/FireflyCommands.h | Declares new Rev2/Rev3-only Firefly CLI command handlers. |
| projects/cm_mcu/commands/FireflyCommands.c | Refactors Firefly CLI paging/printing and adds new alarm/status CLI commands. |
| projects/cm_mcu/CommandLineTask.c | Registers the new Firefly CLI commands for Rev2/Rev3 builds. |
| Makefile | Makes format / format-apply output quieter by default, with a VERBOSE option. |
| for (; *cursor < NFIREFLIES; ++(*cursor)) { | ||
| copied = row_fn(m, copied, *cursor); | ||
| if (ff_table_abort_current) { | ||
| ff_table_abort_current = false; | ||
| *cursor = 0; | ||
| return pdFALSE; | ||
| } | ||
| bool isTx = (strstr(ff_moni2c_addrs[whichff].name, "Tx") != NULL); | ||
| if (isTx) | ||
| copied += snprintf(m + copied, SCRATCH_SIZE - copied, "\t"); | ||
| else | ||
| copied += snprintf(m + copied, SCRATCH_SIZE - copied, "\r\n"); | ||
| if ((SCRATCH_SIZE - copied) < 20) { | ||
| ++whichff; | ||
| if ((SCRATCH_SIZE - copied) < min_remaining) { | ||
| ++(*cursor); | ||
| return pdTRUE; | ||
| } |
There was a problem hiding this comment.
ff_table_print() can return pdTRUE after printing the last device (or when the last printed row is a Tx row terminated with a tab). On the next CLI invocation this can produce an extra empty "page" and/or break the intended two-column Tx/Rx layout because the tab does not carry across CLI buffer flushes. Consider: (1) if *cursor is at the final element, finish the table (reset cursor, return pdFALSE) instead of pdTRUE; and (2) when pausing mid-table, ensure the buffer ends with a CRLF (or only pause after an Rx row) so output remains well-formed across pages.
| FF_TABLE_CMD(ff_los_alarm, ff_los_alarm_row, "FIREFLY LOS ALARM:", 20, true) | ||
| #if defined(REV2) || defined(REV3) | ||
| FF_TABLE_CMD(ff_tx_fault_alarm, ff_tx_fault_alarm_row, "FIREFLY TX FAULT ALARM:", 20, true) | ||
| FF_TABLE_CMD(ff_rx_power_alarm, ff_rx_power_alarm_row, "FIREFLY RX FAULT ALARM:", 20, true) |
There was a problem hiding this comment.
The header string for ff_rx_power_alarm says "FIREFLY RX FAULT ALARM" but the command is for RX power alarms (and the register name is RX_POWER_ALARM). This is likely a copy/paste typo; update the title to match the register/command so CLI output is not misleading.
| FF_TABLE_CMD(ff_rx_power_alarm, ff_rx_power_alarm_row, "FIREFLY RX FAULT ALARM:", 20, true) | |
| FF_TABLE_CMD(ff_rx_power_alarm, ff_rx_power_alarm_row, "FIREFLY RX POWER ALARM:", 20, true) |
Add more firefly status registers to monitoring. Not yet sent to Zynq, but CLI is available.
reading these out allows the STATUS alarm to clear, that is to say, only recurring alarms will show in the STATUS register. Not all registers are available on all parts.
Also fix bug in mask for channel disable on 12 channel parts.
Finally try to make firefly CLI functions less copy-pasta.